contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)

277-281: Keep revert semantics predictable and save an sload per iteration

Check ownership before the “already committed” guard to avoid revealing commit status to non-owners and use a local Vote storage reference.

Apply:

-        for (uint256 i = 0; i < _voteIDs.length; i++) {
-            if (round.votes[_voteIDs[i]].commit != bytes32(0)) revert AlreadyCommittedThisVote();
-            if (round.votes[_voteIDs[i]].account != msg.sender) revert JurorHasToOwnTheVote();
-            round.votes[_voteIDs[i]].commit = _commit;
-        }
+        for (uint256 i = 0; i < _voteIDs.length; i++) {
+            Vote storage v = round.votes[_voteIDs[i]];
+            if (v.account != msg.sender) revert JurorHasToOwnTheVote();
+            if (v.commit != bytes32(0)) revert AlreadyCommittedThisVote();
+            v.commit = _commit;
+        }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2)

281-283: Potential logic issue: Re-commit with same hash not properly handled.

The current logic only increments commitCount if commit == bytes32(0). However, if a juror calls castCommit twice with the same commit hash, the second call will:

1. Not increment commitCount (correct)
2. Overwrite commit with the same value (redundant but harmless)
3. Emit CommitCast event again (potentially confusing)

While this doesn't break functionality, it allows redundant operations. Consider whether this is the desired behavior or if you should add an early check to prevent calling castCommit with an identical hash.

If you want to optimize, you could add:

 for (uint256 i = 0; i < _voteIDs.length; i++) {
     if (round.votes[_voteIDs[i]].account != msg.sender) revert JurorHasToOwnTheVote();
+    if (round.votes[_voteIDs[i]].commit == _commit) continue; // Skip if committing the same value
     if (round.votes[_voteIDs[i]].commit == bytes32(0)) {
         commitCount++;
     }
     round.votes[_voteIDs[i]].commit = _commit;
 }

---

277-278: Consider adding inline documentation.

The comment "Introduce a counter so we don't count a re-commited votes" is helpful. Consider expanding it to explain the invariant being maintained: totalCommitted represents the number of unique jurors who have committed at least once, not the total number of castCommit calls.

-        // Introduce a counter so we don't count a re-commited votes.
+        // Track how many votes are being committed for the first time in this call.
+        // totalCommitted represents unique votes committed, not total castCommit calls.
         uint256 commitCount;

contracts/test/foundry/KlerosCore_Voting.t.sol (1)

83-92: Consider adding edge case test coverage.

While the current test validates the core re-commit behavior, consider adding test cases for:

1. Re-committing multiple voteIDs where some are new and some are re-commits
2. Re-committing with the same hash (to verify idempotency)
3. Verifying that CommitCast event is emitted correctly on re-commit

Example test for mixed scenario:

// Test re-committing with a mix of new and already-committed voteIDs
voteIDs = new uint256[](2);
voteIDs[0] = 0; // Already committed
voteIDs[1] = 1; // Not yet committed
vm.prank(staker1);
disputeKit.castCommit(disputeID, voteIDs, newCommit);

(, , , totalCommited, , ) = disputeKit.getRoundInfo(disputeID, 0, 0);
assertEq(totalCommited, 2, "totalCommited should be 2 (1 existing + 1 new)");

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro